Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add initial integration test suite #371

Merged
merged 17 commits into from
Jul 6, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Jun 29, 2022

Summary

Creates a test harness/structure for integration tests and adds an initial integration test suite to it

Details

  • some additional ts-jest config is needed to parse index.ts

    • since Rollup isn't used when importing files during testing, we need to enable esModuleInterop
      • the import of findCacheDir was immediately erroring without this (and maybe more, but esModuleInterop made the import work)
    • add tsconfig.test.json for this purpose
    • use ts-jest JSDoc types to get types for the ts-jest config as well
  • add an integration/ dir in the __tests__/ dir for all integration tests

    • add a fixtures/ dir in there as well for integration test fixtures
      • add a tsconfig.json for all fixtures to use
        • basically so that rpt2 source code is not included, though also bc this may be good to customize for fixtures
      • add a simple fixture with "no-errors" that does an import and a type-only import
  • add a simple integration test that just ensures that all files are part of the bundle

    • the bundled index file and each file's declaration and declaration map
    • and no other files!
    • for Rollup, need to use paths relative to the root of the whole project
      • probably because that is cwd when running jest from root
      • will probably abstract out helpers here in the future as the test suite grows
    • 70% coverage of index.ts with just this one test!
  • update CONTRIBUTING.md now that an integration test exists

EDIT: added more tests below!

  • properly test the cache (92% coverage)
  • test w/ and w/o declarations + declaration maps
  • test case of plain JS that rpt2 skips and Rollup reads, which still makes it into the bundle
  • test allowJs + emitDeclarationOnly from feat: support emitDeclarationOnly #366
  • test missing/non-existent tsconfig error
  • test semantic and syntactic error
    • plus w/ check: false and abortOnError: false

Review Notes

Creating the harness itself was probably the biggest "mental" blocker for me (mostly in the number of unknowns to solve, but didn't end up being that complex). Now that this is created, I've already started adding more tests to it, namely for edge cases and error cases. Might add some to this PR pending when it gets merged, but otherwise will add those in additional PRs.

  • Also I haven't quite figured out the error testing process for non-fatal errors yet, but I was able to roughly get fix: type-check included files missed by transform (type-only files) #345 tested with this harness as well, which is one reason why I felt more confident in moving that to "ready for review"
    • EDIT: have testing for warnings now
  • EDIT: added more tests in this PR as it has yet to be merged. and this creates a larger harness/structure for different types of tests too. I think only watch mode tests are missing now

- some additional ts-jest config is needed to parse `index.ts`
  - since Rollup isn't used when importing files during testing, we need to enable `esModuleInterop`
    - the import of `findCacheDir` was immediately erroring without this (and maybe more, but `esModuleInterop` made the import work)
  - add `tsconfig.test.json` for this purpose
  - use `ts-jest` JSDoc types to get types for the ts-jest config as well

- add an integration/ dir in the __tests__/ dir for all integration tests
  - add a fixtures/ dir in there as well for integration test fixtures
    - add a `tsconfig.json` for all fixtures to use
      - basically so that rpt2 source code is not `include`d, though also bc this may be good to customize for fixtures
    - add a simple fixture with "no-errors" that does an import and a type-only import

- add a simple integration test that just ensures that all files are part of the bundle
  - the bundled `index` file and each file's declaration and declaration map
  - and no _other_ files!
  - for Rollup, need to use paths relative to the root of the whole project
    - probably because that is `cwd` when running `jest` from root
    - will probably abstract out helpers here in the future as the test suite grows
  - 70% coverage of `index.ts` with just this one test!

- update CONTRIBUTING.md now that an integration test exists
@agilgur5 agilgur5 added scope: docs Documentation could be improved. Or changes that only affect docs kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: tests Tests could be improved. Or changes that only affect tests topic: type-only / emit-less imports Related to importing type-only files that will not be emitted labels Jun 29, 2022
agilgur5 added 2 commits July 4, 2022 10:54
- similar to the unit test suite

- also remove the `.only` that I was using during development
- should probably do this for each integration test suite

- refactor: split out a `genBundle` func to DRY things up
  - this was bound to happen eventually
    - don't think testing other output formats is necessary since we don't have any specific logic for that, so that is just Rollup behavior
  - take `input` path and rpt2 options as params
    - and add the `tsconfig` as a default
  - add a hacky workaround so that Rollup doesn't output a bunch of warnings in the logs
  - format: use double quotes for strings consistently (my bad, I normally use single quotes in my own repos)
@agilgur5 agilgur5 force-pushed the test-add-integration-tests branch from b519f93 to 1710a99 Compare July 4, 2022 15:38
agilgur5 added 6 commits July 4, 2022 11:48
- so we can clean it up after testing to ensure consistency
  - and so we're not filling up the cache in `node_modules` with testing caches

- also add a comment to one the of the tests
- make sure they don't get output when not asked for
- `clean: true` also means that no cache is created
  - which is a bit confusing naming, but was requested by a few users and implemented in f15cb84
  - so have to create the bundle one _more_ time to create the cache
    - note that `tscache`'s `getCached` and `isDirty` functions are now actually covered in the coverage report
- double it bc it was occassionally failing on CI due to timeouts
- ensures that TS understands the JS w/ DTS w/o error
- and that rpt2 filters out JS while Rollup still resolves it on its own (since Rollup understands ESM)
  - similar to testing w/ a different plugin (i.e. literally testing an "integration"), but this is native Rollup behavior in this case where it doesn't need a plugin to understand ESM
- also the first test of actual code contents

- reformat the cache test a bit into its own block since we now have ~3 different sets of tests in the suite
- this doesn't test a new code path and the first test already tests the entire bundle for the fixture, so the other tests just repeat that
@agilgur5 agilgur5 force-pushed the test-add-integration-tests branch from 874270e to efced2b Compare July 4, 2022 17:41
- refactor: rename `integration/index.spec` -> `integration/no-errors.spec`
- refactor: split `genBundle` func out into a helper file to be used by multiple integration test suites
  - simplify the `genBundle` within `no-errors` as such

- create new `errors` fixture just with some simple code that doesn't type-check for now
  - add test to check that the exact semantic error pops up
    - and it even tests colors 😮  ...though I was confused for some time why the strings weren't matching... 😐
  - add test to make sure it doesn't pop up when `check: false` or `abortOnError: false`
    - when `abortOnError: false`, detect that a warning is created instead

- due to the new `errors` dir in the fixtures dir, the fixtures `tsconfig` now picks up two dirs
  - which changes the `rootDir` etc
  - so create two tiny `tsconfig`s in both fixture dirs that just extend that one
    - and now tests all run simiarly to before
@agilgur5 agilgur5 changed the title test: add initial integration test harness test: add initial integration test suite Jul 4, 2022
agilgur5 added 3 commits July 4, 2022 16:34
- this too timed out, probably due to the checks that _don't_ error
- so that Jest can parallelize these
  - CI was timing out on this, so splitting it out should help as it'll be 10s each
- bc it was still timing out in some cases 😕
  - this time the `no-errors` suite was timing out, so just increase all around
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jul 4, 2022

I've added a handful more integration tests now, so this is now a proper integration test suite, and not just a simple one-test harness anymore. Edited the title and description of the PR to match.

Had to bump test timeouts a couple times since integration tests take a good bit longer, especially when comparing two builds against each other (e.g. for cache / no cache).

A few more areas to add tests to:

But this PR is (still) ready to review/merge, those are extras and can be done in future PRs (same as the tests I added today on top of the simple harness from a week ago)

agilgur5 added 2 commits July 4, 2022 17:28
- woooops... was wondering why it was `.ts`; turns out because I wrote the `output.file` setting that way 😅 😅 😅

- also add a missing comment in errors for consistency
- and put code checks _after_ file structure checks, since that's a deeper check
- should output declaration and declaration map for file
  - code + declaration should contain the one-line function
  - code _shouldn't_ contain anything from TS files

- since this is plain ESM code, we don't need another plugin to process this
  - nice workaround to installing another plugin that I hadn't thought of till now!
@agilgur5 agilgur5 force-pushed the test-add-integration-tests branch from 60d2e5f to 172c32f Compare July 4, 2022 23:59
agilgur5 added 2 commits July 4, 2022 20:01
- add a file to `errors` with a syntax error in it
  - apparently this does throw syntax err, but does not cause `emitSkipped: true`... odd...
    - so may need another test for that...
    - `abortOnError: false` / `check: false` both cause Rollup to error out instead
  - rename `errors/index` -> `errors/semantic` since we have different kinds now
  - change the `include` to only take a single file, so that we don't unnecessarily generate declarations or type-check other error files

- refactor(test): rewrite `genBundle` in both files to take a relative path to a file
  - simplifies the `emitDeclarationOnly` test as we can now just reuse the local `genBundle` instead of calling `helpers.genBundle` directly
  - use the same structure for `errors`'s different files as well

- refactor(test): split `errors.spec` into `tsconfig` errors, semantic errors, and syntactic errors (for now)

- add a slightly hacky workaround to get `fs.remove` to not error
  - seems to be a race condition due to the thrown errors and file handles not being closed immediately on throw (seems like they close during garbage collection instead?)
  - see linked issue in the comment; workaround is to just give it some more time
    - not sure that there is a true fix for this, since an improper close may cause indeterminate behavior
- got a Windows error in CI for the `errors` test suite
@agilgur5 agilgur5 force-pushed the test-add-integration-tests branch from 172c32f to 62c3b85 Compare July 5, 2022 00:07
@agilgur5 agilgur5 added topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs labels Jul 5, 2022
@ezolenko
Copy link
Owner

ezolenko commented Jul 6, 2022

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: docs Documentation could be improved. Or changes that only affect docs scope: tests Tests could be improved. Or changes that only affect tests topic: OS separators Path normalization between OSes. Windows = '\', POSIX = '/'. Rollup resolve = native, TS = POSIX topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs topic: type-only / emit-less imports Related to importing type-only files that will not be emitted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants